Repository navigation
feat(review): capture reviewer results against the exact provider-issued slot - #274
Alan-TheGentleman wants to merge 3 commits into
Conversation
providerReviewerProjection already validated every review.capture-result binding field but the code lived inline in extensions/gentle-ai.ts. Extract it into lib/review-result-capture.ts as deriveCaptureSlots, the single reusable validator Wave 1's FINALIZE capture phase will call, plus assertLensSlotBijection for the lens/slot fail-closed check.
…lt staging Pi ran review lenses but never captured their documents through native admission: providerReviewerProjection validated the review.capture-result binding and then threw the work away, and NativeReviewCliV216.finalize() staged lensResults into a tmp file no argv ever consumed. Route providerReviewerProjection through deriveCaptureSlots so exactly one validator exists, and add a FINALIZE capture phase that, for every review_result.lens_results entry: fails closed on any lens/slot mismatch before capturing anything, calls captureResult() with the candidate root as cwd and the provider's own argument tokens forwarded verbatim in ascending selectedOrder, and re-checks each admitted manifest against its slot's subject hash and lens. Transport is selected per run: every admitted manifest carrying a path selects resultArtifactFiles in ascending order, any manifest carrying a reference selects capturedResults instead, and the retired --result flag is never emitted. Also removes the dead lensResults staging from NativeReviewCliV216.finalize (the negotiated production client) and regenerates its runtime mirror. The legacy plain-CLI NativeReviewCliV214 keeps its own working --result staging untouched, since Wave 1 only targets the negotiated FINALIZE transport.
Records RED/GREEN evidence and the two scoped deviations (bijection checks only outstanding slots until W2's record map exists; the dead lensResults staging deletion is scoped to NativeReviewCliV216, not the still-used legacy NativeReviewCliV214 client) for all 14 W1 tasks.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This capture-slot extraction is useful, but this PR is not mergeable as the new Pi provider path. Gentle AI provider tasks now require a Go-issued opaque host task, while this branch still participates in Pi-owned reviewer, refuter, or validator document construction. The canonical parity contract is now #311. Its provider prerequisite is explicit Pi or generic host-relay support from Gentle AI. Once that released contract exists, the slot-validation extraction here can be split into a narrow transport slice, but no part of this PR may preserve local role semantics, use the OpenCode relay, or pass Pi as Keep this branch blocked until #311 P1 through P4 have a released provider schema and capability to target. Then rebase only the reusable capture-slot work and delete the local semantic FINALIZE path. |
|
Closing as obsoleted by the atomic review lifecycle: this wave1 chain instruments the pre-atomic relay protocol (slot capture, bounded relaunch, and lost-output recovery). The atomic relay (#386) reworked capture and closure ownership, and upstream gentle-ai main has since removed FINALIZE entirely (close on the last causal event), so the recovery and diagnostics surfaces this chain hardens no longer exist in the current model. Anything still relevant should restart as fresh slices against the atomic relay. |
1 participant
First slice of Wave 1, the #2028 host behavior. Independent of the release-artifact chain; only W3 depends on that foundation.
The defect was narrower than it looked
providerReviewerProjectionalready validated every binding field — lineage, authority revision, target, base and candidate trees, changed-path manifest hash, lens, order, subject hash, including argument-token equality and duplicate detection. Then it threw the result away.So this is an extraction, not a new validator. Writing a second one would have created exactly the competing provider authority the plan forbids.
deriveCaptureSlotsis now the single six-field validator; both the projection and the new FINALIZE capture phase call it.Behavior preservation was verified by running the pre-existing 320-test suite unchanged across the refactor step alone, before anything new was added.
Pi transports, it does not interpret
Argument tokens stay discrete argv elements, byte-identical, no shell, fixed executable. Capture cwd is
candidateView.root. Tokens carrying--repository-contextalongside an explicit cwd are rejected, and empty or non-string tokens are refused.An admitted-manifest path containing
.., an absolute path, or an executable-looking name is forwarded byte-identical with zero Pi-side filesystem access. Pi is not in the business of deciding what a provider path means. A manifest carrying bothpathandreferenceis refused outright.Dead code, and a correction to the task text
The task said to drop
resultFilesandlensResultsfromNativeFinalizeRequest. That would have broken the legacy plain-CLI client, which genuinely stages and passes them as--resultargv and is still exercised by roughly twenty fixture-compatibility tests.The provably dead staging was in the negotiated production client only, where documents were written to a 0600 tmp file that its argv never referenced. That is what was deleted. The shared type keeps the fields, with a comment naming who still uses them.
One thing found along the way
captureResult()existed on the production client but was never declared on theNativeReviewCliinterface, so no interface-typed caller could reach it. Declared here.Transport selection is per run
Every slot carrying
pathselects the artifact-file list in ascending order. Anyreference, or any already-committed slot, selects the captured-results flag. The retired--resultis never emitted from the negotiated path.Tests
17 new pure-function tests plus new bijection, cwd, verbatim-token, transport-selection and documentation-like-path coverage. 1022 pass, 3 fail — the receipt-driven-development-disabled failures, confirmed identical against a stashed baseline.
check:transaction-runnergreen with the runtime regenerated rather than hand-edited.Receipt-driven development stays disabled throughout; nothing here starts, recovers, retries or reclaims review authority.
Rollback
The two feature commits revert together, restoring the prior FINALIZE argument mapping. The provider binary is untouched.